test/solidity/Facets/DexManagerFacet.t.sol (1)

273-274: Consider using unchecked block for gas optimization.

The loop counters in these test functions could use unchecked blocks for gas optimization, similar to the pattern used in the existing test on line 128.

Apply this diff to optimize the loops:

-        for (uint256 i = 0; i < 3; i++) {
+        for (uint256 i = 0; i < 3; ) {
             assertTrue(dexMgr.isFunctionApproved(signatures[i]));
+            unchecked { ++i; }
         }

-        for (uint256 i = 0; i < 3; i++) {
+        for (uint256 i = 0; i < 3; ) {
             assertFalse(dexMgr.isFunctionApproved(signatures[i]));
+            unchecked { ++i; }
         }

Also applies to: 278-279

test/solidity/Facets/DeBridgeDlnFacet.t.sol (1)

411-430: LGTM! Good low-level storage testing.

The test effectively validates storage slot computation and access. Consider adding a comment explaining the storage layout pattern being tested.

Add a comment explaining the diamond storage pattern:

 function test_getStorage() public {
+    // Test diamond storage pattern where each facet's storage is namespaced
+    // to avoid collisions with other facets' storage variables
     bytes32 namespace = keccak256("com.lifi.facets.debridgedln");

test/solidity/Facets/AmarokFacetPacked.t.sol (1)

416-513: LGTM! Comprehensive chain ID mapping tests.

The test suite thoroughly covers chain ID mappings for all major chains and handles invalid cases. Consider refactoring to use parameterized testing for better maintainability.

Consider refactoring the chain-specific tests into a parameterized test:

struct ChainTestCase {
    string name;
    uint32 domainId;
    uint32 expectedChainId;
}

function test_getChainIdForDomain() public {
    ChainTestCase[] memory testCases = new ChainTestCase[](7);
    testCases[0] = ChainTestCase("ETH", 6648936, 1);
    testCases[1] = ChainTestCase("POL", 1886350457, 137);
    // ... add other cases

    for (uint i = 0; i < testCases.length; i++) {
        ChainTestCase memory tc = testCases[i];
        uint32 result = amarokFacetPacked.getChainIdForDomain(tc.domainId);
        assertEq(
            result,
            tc.expectedChainId,
            string.concat(tc.name, " domainId should return correct chainId")
        );
    }
}

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

test/solidity/Facets/AcrossFacetV3.t.sol (1)

276-293: 🛠️ Refactor suggestion

Rename test function to follow the naming convention.

Based on the past review comments, the test name should follow the convention: testRevert_willRevertWhen<Scenario>. A better name would be:

-    function testRevert_InformationMismatch() public {
+    function testRevert_willRevertWhenReceiverAddressesMismatch() public {

Also, consider adding a descriptive comment explaining the test scenario.

test/solidity/Facets/CBridge.t.sol (1)

206-227: 🛠️ Refactor suggestion

Add balance checks to verify token transfers.

Based on past review comments, when testing token transfers, it's important to verify balance changes in the calling wallet.

Add balance checks before and after the refund:

 function test_TriggerRefundSucceedsWhenCalledByOwner() public {
     address callTo = address(CBRIDGE_ROUTER);
     bytes memory callData = abi.encodeWithSignature("someFunction()");
     address assetAddress = ADDRESS_USDT;
     address to = USER_RECEIVER;
     uint256 amount = 100 * 10 ** usdt.decimals();

     deal(ADDRESS_USDT, address(cBridge), amount);
+    uint256 preRefundBalance = IERC20(assetAddress).balanceOf(to);

     vm.mockCall(callTo, callData, abi.encode(true));

     vm.expectEmit(true, true, true, true, address(cBridge));
     emit CBridgeRefund(assetAddress, to, amount);

     cBridge.triggerRefund(
         payable(callTo),
         callData,
         assetAddress,
         to,
         amount
     );
+    
+    uint256 postRefundBalance = IERC20(assetAddress).balanceOf(to);
+    assertEq(postRefundBalance - preRefundBalance, amount, "Refund amount should be correct");
 }

test/solidity/Facets/CBridge.t.sol (1)

251-269: Test name should be more specific about why the address is not allowed.

Based on past review comments, test names should be specific to differentiate between various failing scenarios.

Rename the test to be more specific:

-function testRevert_TriggerRefundFailsWhenCallToAddressIsNotAllowed()
+function testRevert_TriggerRefundFailsWhenCallToAddressIsNotInAllowList()

test/solidity/Facets/CBridgeFacetPacked.t.sol (1)

517-535: Test name should be consistent with CBridge.t.sol.

For consistency, rename this test to match the suggested name in CBridge.t.sol.

Rename the test:

-function testRevert_TriggerRefundFailsWhenCallToAddressIsNotAllowed()
+function testRevert_TriggerRefundFailsWhenCallToAddressIsNotInAllowList()

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

test/solidity/Facets/CBridge.t.sol (1)

206-227: 🛠️ Refactor suggestion

Add balance checks to verify token transfer.

The test should verify the token balances before and after the refund operation to ensure the transfer was successful.

 function test_TriggerRefundSucceedsWhenCalledByOwner() public {
     address callTo = address(CBRIDGE_ROUTER);
     bytes memory callData = abi.encodeWithSignature("someFunction()");
     address assetAddress = ADDRESS_USDT;
     address to = USER_RECEIVER;
     uint256 amount = 100 * 10 ** usdt.decimals();

     deal(ADDRESS_USDT, address(cBridge), amount);
+    uint256 initialBalance = IERC20(assetAddress).balanceOf(to);
+    uint256 initialContractBalance = IERC20(assetAddress).balanceOf(address(cBridge));

     vm.mockCall(callTo, callData, abi.encode(true));

     vm.expectEmit(true, true, true, true, address(cBridge));
     emit CBridgeRefund(assetAddress, to, amount);

     cBridge.triggerRefund(
         payable(callTo),
         callData,
         assetAddress,
         to,
         amount
     );
+
+    assertEq(
+        IERC20(assetAddress).balanceOf(to),
+        initialBalance + amount,
+        "Recipient balance should increase by refund amount"
+    );
+    assertEq(
+        IERC20(assetAddress).balanceOf(address(cBridge)),
+        initialContractBalance - amount,
+        "Contract balance should decrease by refund amount"
+    );
 }

test/solidity/Facets/CBridge.t.sol (1)

271-289: Enhance mock call setup for failure test.

The test should explicitly mock the call to return false or revert to better simulate the failure scenario.

 function testRevert_TriggerRefundFailsWhenCallToCBridgeRouterFails() public {
     address callTo = CBRIDGE_ROUTER;
     bytes memory callData = abi.encodeWithSignature("someFunction()");
     address assetAddress = ADDRESS_USDT;
     address to = USER_RECEIVER;
     uint256 amount = 100 * 10 ** usdt.decimals();

+    vm.mockCall(
+        callTo,
+        callData,
+        abi.encode(false)  // Simulate failure by returning false
+    );
+
     vm.expectRevert(ExternalCallFailed.selector);

     cBridge.triggerRefund(
         payable(callTo),
         callData,
         assetAddress,
         to,
         amount
     );
 }

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

test/solidity/Facets/AmarokFacetPacked.t.sol (1)

416-476: Consider parameterizing test cases to improve maintainability.

The test cases for domain-to-chain ID mapping could be refactored to avoid duplicate code structure, as suggested in previous reviews.

Consider this approach:

-    function test_getChainIdForValidDomains() public {
-        DomainChainTestCase[] memory testCases = new DomainChainTestCase[](8);
-        testCases[0] = DomainChainTestCase({
-            domainId: 6648936,
-            expectedChainId: 1,
-            description: "ETH domainId should return chainId 1"
-        });
-        // ... more test cases ...
-        for (uint256 i = 0; i < testCases.length; i++) {
-            uint32 result = amarokFacetPacked.getChainIdForDomain(
-                testCases[i].domainId
-            );
-            assertEq(
-                result,
-                testCases[i].expectedChainId,
-                testCases[i].description
-            );
-        }
-    }
+    function _assertDomainChainMapping(
+        uint32 domainId,
+        uint32 expectedChainId,
+        string memory description
+    ) internal {
+        uint32 result = amarokFacetPacked.getChainIdForDomain(domainId);
+        assertEq(result, expectedChainId, description);
+    }
+
+    function test_getChainIdForValidDomains() public {
+        _assertDomainChainMapping(6648936, 1, "ETH domainId should return chainId 1");
+        _assertDomainChainMapping(1886350457, 137, "POL domainId should return chainId 137");
+        _assertDomainChainMapping(6450786, 56, "BSC domainId should return chainId 56");
+        _assertDomainChainMapping(1869640809, 10, "OPT domainId should return chainId 10");
+        _assertDomainChainMapping(6778479, 100, "GNO domainId should return chainId 100");
+        _assertDomainChainMapping(1634886255, 42161, "ARB domainId should return chainId 42161");
+        _assertDomainChainMapping(1818848877, 59144, "LIN domainId should return chainId 59144");
+        _assertDomainChainMapping(9999999, 0, "Unknown domainId should return 0");
+    }

test/solidity/Facets/AccessManagerFacet.t.sol (1)

87-208: Great addition of comprehensive test coverage!

The new test functions thoroughly cover various access control scenarios including:

• Self-authorization prevention
• Owner-only access
• Default access state
• Access granting/revocation
• Different method selectors and executors

Consider adding additional assertions for completeness.

Some test functions could benefit from additional assertions to verify the complete state. For example:

 function test_CanCheckGrantedAccess() public {
     vm.startPrank(USER_DIAMOND_OWNER);

     accessMgr.setCanExecute(
         RestrictedContract.restrictedMethod.selector,
         address(0xb33f),
         true
     );

     bool canExecute = accessMgr.addressCanExecuteMethod(
         RestrictedContract.restrictedMethod.selector,
         address(0xb33f)
     );

     assertEq(canExecute, true, "Access should be granted");
+    // Verify the actual method execution succeeds
+    vm.prank(address(0xb33f));
+    bool result = restricted.restrictedMethod();
+    assertEq(result, true, "Method execution should succeed");

     vm.stopPrank();
 }

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

test/solidity/Facets/CBridgeFacetPacked.t.sol (1)

601-623: Fix typo in function name.

The function name contains a typo: "Diffrent" should be "Different".

-    function testRevert_TriggerRefundFailsWhenTryingToCallDiffrentContractThanCBridgeRouter()
+    function testRevert_TriggerRefundFailsWhenTryingToCallDifferentContractThanCBridgeRouter()

test/solidity/Facets/CBridge.t.sol (2)

306-306: Fix typo in function name.

"Diffrent" should be "Different" in the function name.

-    function testRevert_TriggerRefundFailsWhenTryingToCallDiffrentContractThanCBridgeRouter()
+    function testRevert_TriggerRefundFailsWhenTryingToCallDifferentContractThanCBridgeRouter()

---

212-214: Use descriptive mock function name.

The mock function name "someFunction()" is not descriptive of what it's simulating. Consider using a more specific name that reflects the actual CBridge refund functionality being tested.

-        bytes memory callData = abi.encodeWithSignature("someFunction()");
+        bytes memory callData = abi.encodeWithSignature("withdrawRefund(uint256,address)");

Also applies to: 251-253, 287-289, 311-313, 335-337

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

test/solidity/Facets/DexManagerFacet.t.sol (2)

293-295: Optimize loop using unchecked increment.

The loop counter can safely use unchecked increment since the loop has a fixed size of 3 iterations.

Apply this diff to optimize the loop:

-        for (uint256 i = 0; i < 3; i++) {
+        for (uint256 i = 0; i < 3;) {
             assertTrue(dexMgr.isFunctionApproved(signatures[i]));
+            unchecked { ++i; }
         }

---

216-227: Improve test structure by combining vm.prank and vm.expectRevert.

The test structure can be improved by placing vm.expectRevert before vm.prank to make the test flow clearer.

Apply this diff to improve the test structure:

     function testRevert_FailToRemoveDexFromNotOwner() public {
         vm.prank(USER_DIAMOND_OWNER);
         dexMgr.addDex(address(c1));
         vm.stopPrank();

-        vm.expectRevert(UnAuthorized.selector);
-
         vm.prank(NOT_DIAMOND_OWNER);
+        vm.expectRevert(UnAuthorized.selector);
         dexMgr.removeDex(address(c1));
     }

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

test/solidity/Facets/AccessManagerFacet.t.sol (1)

87-208: LGTM! Comprehensive test coverage for access control scenarios.

The new test functions:

• Cover all critical access control paths
• Follow consistent patterns and naming conventions
• Improve overall test coverage as per PR objectives

One suggestion to consider: Add test cases for edge cases such as:

• Setting access for address(0)
• Setting access for multiple methods in a single transaction

test/solidity/Facets/DeBridgeDlnFacet.t.sol (2)

365-377: Consider adding more edge cases to the initialization test.

While the test correctly verifies the NotInitialized error case, consider adding tests for:

• Attempting to set chain ID with value 0
• Attempting to set deBridge chain ID with value 0

 function testRevert_FailsToSetDeBridgeChainIdIfNotInitialized() public {
     vm.startPrank(address(0));
     
     TestDeBridgeDlnFacet uninitializedFacet = new TestDeBridgeDlnFacet(
         DLN_SOURCE
     );
     
     vm.expectRevert(NotInitialized.selector);
     uninitializedFacet.setDeBridgeChainId(137, 1001);
     
+    // Test with chain ID 0
+    vm.expectRevert(NotInitialized.selector);
+    uninitializedFacet.setDeBridgeChainId(0, 1001);
+    
+    // Test with deBridge chain ID 0
+    vm.expectRevert(NotInitialized.selector);
+    uninitializedFacet.setDeBridgeChainId(137, 0);
+    
     vm.stopPrank();
 }

---

423-442: Consider adding boundary tests for storage.

The storage test is well-implemented but could benefit from additional edge cases:

• Test with maximum uint256 value
• Test with zero value

 function test_GetStorage() public {
     bytes32 namespace = keccak256("com.lifi.facets.debridgedln");
     
     uint256 chainIdKey = 1;
     uint256 expectedValue = 42;
     
     // compute the correct storage slot for `deBridgeChainId[chainIdKey]`
     bytes32 storageSlot = keccak256(abi.encode(chainIdKey, namespace));
     
     // store the test value in the computed slot
     vm.store(
         address(deBridgeDlnFacet),
         storageSlot,
         bytes32(expectedValue)
     );
     
     uint256 storedValue = deBridgeDlnFacet.getDeBridgeChainId(chainIdKey);
     
     assertEq(storedValue, expectedValue);
+    
+    // Test with maximum uint256 value
+    uint256 maxValue = type(uint256).max;
+    vm.store(
+        address(deBridgeDlnFacet),
+        storageSlot,
+        bytes32(maxValue)
+    );
+    storedValue = deBridgeDlnFacet.getDeBridgeChainId(chainIdKey);
+    assertEq(storedValue, maxValue);
+    
+    // Test with zero value
+    vm.store(
+        address(deBridgeDlnFacet),
+        storageSlot,
+        bytes32(uint256(0))
+    );
+    storedValue = deBridgeDlnFacet.getDeBridgeChainId(chainIdKey);
+    assertEq(storedValue, 0);
 }

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

test/solidity/Facets/DeBridgeDlnFacet.t.sol (1)

411-459: Consider adding edge cases to error handling tests.

While the error handling tests cover the basic scenarios, consider adding tests for:

• Setting chain ID to zero
• Setting the same chain ID multiple times
• Setting chain ID with max uint256 values

Example test case:

+    function testRevert_FailsIfAttemptingToSetZeroChainId() public {
+        vm.startPrank(USER_DIAMOND_OWNER);
+
+        uint256 chainId = 137;
+        uint256 deBridgeChainId = 0;
+
+        vm.expectRevert("ZeroChainId");
+
+        deBridgeDlnFacet.setDeBridgeChainId(chainId, deBridgeChainId);
+
+        vm.stopPrank();
+    }

test/solidity/Facets/DexManagerFacet.t.sol (3)

9-9: Add documentation for error types and constants.

Consider adding NatSpec comments to document:

• The purpose of imported error types
• The significance of the NOT_DIAMOND_OWNER constant

+/// @notice Error types for authorization and validation checks
 import { CannotAuthoriseSelf, UnAuthorized, InvalidContract } from "src/Errors/GenericErrors.sol";

+/// @notice Address used for negative test cases, representing a non-owner account
 address internal constant NOT_DIAMOND_OWNER = address(0xabc123456);

Also applies to: 16-16

---

303-305: Use unchecked block for loop counter.

For consistency with other loops in the file (e.g., lines 129-134), consider using an unchecked block for the loop counter increment.

-        for (uint256 i = 0; i < 3; i++) {
-            assertTrue(dexMgr.isFunctionApproved(signatures[i]));
-        }
+        for (uint256 i = 0; i < 3; ) {
+            assertTrue(dexMgr.isFunctionApproved(signatures[i]));
+            unchecked {
+                ++i;
+            }
+        }

---

233-237: Maintain consistent ordering of vm.expectRevert and vm.prank calls.

For consistency with other test functions, consider moving vm.prank before vm.expectRevert.

-        vm.expectRevert(UnAuthorized.selector);
-
-        vm.prank(NOT_DIAMOND_OWNER);
+        vm.prank(NOT_DIAMOND_OWNER);
+
+        vm.expectRevert(UnAuthorized.selector);
+
         dexMgr.removeDex(address(c1));

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

test/solidity/Facets/DexManagerFacet.t.sol (5)

50-58: Consider adding negative assertions.

While the test verifies successful DEX addition, it would be more robust to also assert that non-added DEXs are not accidentally approved.

 function test_SucceedsIfOwnerAddsDex() public {
     vm.startPrank(USER_DIAMOND_OWNER);

     dexMgr.addDex(address(c1));
     address[] memory approved = dexMgr.approvedDexs();
     assertEq(approved[0], address(c1));
+    assertEq(approved.length, 1);
+    assertFalse(dexMgr.approvedDexs().contains(address(c2)));

     vm.stopPrank();
 }

---

227-238: Maintain consistent prank patterns.

The test uses individual prank calls instead of the startPrank/stopPrank pattern used in other tests. This inconsistency makes the code harder to maintain.

 function testRevert_FailsIfNonOwnerTriesToRemoveDex() public {
-    vm.prank(USER_DIAMOND_OWNER);
+    vm.startPrank(USER_DIAMOND_OWNER);
     dexMgr.addDex(address(c1));
-    vm.stopPrank();
+    vm.stopPrank();

+    vm.startPrank(NOT_DIAMOND_OWNER);
     vm.expectRevert(UnAuthorized.selector);
-    vm.prank(NOT_DIAMOND_OWNER);
     dexMgr.removeDex(address(c1));
+    vm.stopPrank();
 }

---

241-252: Apply consistent prank pattern here as well.

Similar to the previous test, this one should follow the same startPrank/stopPrank pattern.

 function testRevert_FailsIfNonOwnerTriesToBatchRemoveDex() public {
     address[] memory dexs = new address[](2);
     dexs[0] = address(c1);
     dexs[1] = address(c2);

-    vm.prank(USER_DIAMOND_OWNER);
+    vm.startPrank(USER_DIAMOND_OWNER);
     dexMgr.batchAddDex(dexs);
+    vm.stopPrank();

+    vm.startPrank(NOT_DIAMOND_OWNER);
     vm.expectRevert(UnAuthorized.selector);
-    vm.prank(NOT_DIAMOND_OWNER);
     dexMgr.batchRemoveDex(dexs);
+    vm.stopPrank();
 }

---

304-306: Optimize gas usage in test loops.

For consistency with other loops in the codebase and to optimize gas usage, consider using unchecked increment.

-        for (uint256 i = 0; i < 3; i++) {
+        for (uint256 i = 0; i < 3;) {
             assertTrue(dexMgr.isFunctionApproved(signatures[i]));
+            unchecked { ++i; }
         }

---

309-311: Apply the same gas optimization here.

For consistency, apply the same unchecked increment pattern here.

-        for (uint256 i = 0; i < 3; i++) {
+        for (uint256 i = 0; i < 3;) {
             assertFalse(dexMgr.isFunctionApproved(signatures[i]));
+            unchecked { ++i; }
         }

test/solidity/Facets/AccessManagerFacet.t.sol (1)

170-191: Consider parameterizing the test with different method selectors.

The test could be enhanced by testing multiple different method selectors to ensure consistent behavior.

-    function test_DifferentMethodSelectorReturnsFalse() public {
+    function test_DifferentMethodSelectorReturnsFalse(bytes4 differentSelector) public {
+        vm.assume(differentSelector != RestrictedContract.restrictedMethod.selector);
         vm.startPrank(USER_DIAMOND_OWNER);
 
         accessMgr.setCanExecute(
             RestrictedContract.restrictedMethod.selector,
             address(0xb33f),
             true
         );
 
         vm.stopPrank();
 
         bool canExecute = accessMgr.addressCanExecuteMethod(
-            bytes4(keccak256("anotherMethod()")),
+            differentSelector,
             address(0xb33f)
         );
 
         assertEq(
             canExecute,
             false,
             "Different method selector should return false"
         );
     }

test/solidity/Facets/CBridge.t.sol (2)

306-328: Consider parameterizing the contract address test.

The test could be enhanced by testing multiple different invalid contract addresses.

-    function testRevert_FailsWhenTriggerRefundTryingToCallDiffrentContractThanCBridgeRouter()
+    function testRevert_FailsWhenTriggerRefundTryingToCallDiffrentContractThanCBridgeRouter(address invalidContract)
         public
     {
+        vm.assume(invalidContract != CBRIDGE_ROUTER);
         vm.startPrank(USER_DIAMOND_OWNER);
 
-        address callTo = address(0xdeadbeef);
+        address callTo = invalidContract;
         bytes memory callData = abi.encodeWithSignature("someFunction()");
         address assetAddress = ADDRESS_USDT;

---

330-352: Consider testing with different failure scenarios.

The test could be enhanced by testing different types of external call failures.

     function testRevert_FailsWhenTriggerRefundCallToCBridgeRouterFails()
         public
     {
         vm.startPrank(USER_DIAMOND_OWNER);
 
         address callTo = CBRIDGE_ROUTER;
-        bytes memory callData = abi.encodeWithSignature("someFunction()");
+        bytes[] memory failureCases = new bytes[](3);
+        failureCases[0] = abi.encodeWithSignature("nonexistentFunction()");
+        failureCases[1] = abi.encodeWithSignature("revertingFunction()");
+        failureCases[2] = abi.encodeWithSignature("outOfGasFunction()");
+
+        for(uint i = 0; i < failureCases.length; i++) {
+            vm.expectRevert(ExternalCallFailed.selector);
+            cBridge.triggerRefund(
+                payable(callTo),
+                failureCases[i],
+                assetAddress,
+                to,
+                amount
+            );
+        }

test/solidity/Facets/DeBridgeDlnFacet.t.sol (1)

422-476: Consider adding edge cases to the chain ID tests.

While the test covers main scenarios, consider adding edge cases:

• Maximum possible domain ID
• Domain ID 0
• Sequential domain IDs

     function test_CanGetChainIdForValidDomains() public {
-        DomainChainTestCase[] memory testCases = new DomainChainTestCase[](8);
+        DomainChainTestCase[] memory testCases = new DomainChainTestCase[](11);
         // ... existing test cases ...
+        testCases[8] = DomainChainTestCase({
+            domainId: type(uint32).max,
+            expectedChainId: 0,
+            description: "Max domain ID should return 0"
+        });
+        testCases[9] = DomainChainTestCase({
+            domainId: 0,
+            expectedChainId: 0,
+            description: "Domain ID 0 should return 0"
+        });
+        testCases[10] = DomainChainTestCase({
+            domainId: 1,
+            expectedChainId: 0,
+            description: "Sequential domain ID should return 0"
+        });

test/solidity/Facets/AmarokFacetPacked.t.sol (1)

422-476: Consider parameterizing the domain ID test.

The test could be enhanced by using property-based testing for domain IDs.

-    function test_CanGetChainIdForValidDomains() public {
+    function test_CanGetChainIdForValidDomains(uint32 domainId) public {
+        vm.assume(domainId > 0);
+        
+        // Create mapping of known domain IDs to chain IDs
+        uint32[2][] memory knownMappings = new uint32[2][](7);
+        knownMappings[0] = [uint32(6648936), uint32(1)];  // ETH
+        knownMappings[1] = [uint32(1886350457), uint32(137)];  // POL
+        // ... add other mappings ...
+        
+        bool isKnown = false;
+        uint32 expectedChainId = 0;
+        
+        // Check if the domain ID is known
+        for(uint i = 0; i < knownMappings.length; i++) {
+            if(knownMappings[i][0] == domainId) {
+                isKnown = true;
+                expectedChainId = knownMappings[i][1];
+                break;
+            }
+        }
+        
+        uint32 result = amarokFacetPacked.getChainIdForDomain(domainId);
+        
+        if(isKnown) {
+            assertEq(result, expectedChainId, "Known domain ID returned wrong chain ID");
+        } else {
+            assertEq(result, 0, "Unknown domain ID should return 0");
+        }

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

test/solidity/Facets/DeBridgeDlnFacet.t.sol (2)

44-57: Consider adding a constant for the function selectors array size.

The magic number 7 in the array size initialization could be replaced with a constant for better maintainability.

+    uint256 private constant FUNCTION_SELECTORS_COUNT = 7;
-    bytes4[] memory functionSelectors = new bytes4[](7);
+    bytes4[] memory functionSelectors = new bytes4[](FUNCTION_SELECTORS_COUNT);

---

411-459: Consider adding edge case tests for chain ID values.

While the current tests cover the main functionality, consider adding tests for:

• Maximum uint256 values
• Special chain IDs (e.g., 0, 1)

+    function test_CanSetSpecialChainIds() public {
+        vm.startPrank(USER_DIAMOND_OWNER);
+
+        // Test with chain ID 0
+        deBridgeDlnFacet.setDeBridgeChainId(0, 42);
+        assertEq(deBridgeDlnFacet.getDeBridgeChainId(0), 42);
+
+        // Test with max uint256
+        uint256 maxChainId = type(uint256).max;
+        deBridgeDlnFacet.setDeBridgeChainId(maxChainId, 42);
+        assertEq(deBridgeDlnFacet.getDeBridgeChainId(maxChainId), 42);
+
+        vm.stopPrank();
+    }

test/solidity/Facets/DexManagerFacet.t.sol (5)

10-10: Remove duplicate import of OnlyContractOwner.

The error type OnlyContractOwner is imported twice in the same line.

-import { InvalidContract, OnlyContractOwner, CannotAuthoriseSelf, UnAuthorized, OnlyContractOwner } from "lifi/Errors/GenericErrors.sol";
+import { InvalidContract, OnlyContractOwner, CannotAuthoriseSelf, UnAuthorized } from "lifi/Errors/GenericErrors.sol";

---

315-317: Optimize loop increment for gas efficiency.

For consistency with other similar loops in the file and gas optimization, consider using unchecked increment.

-        for (uint256 i = 0; i < 3; i++) {
+        for (uint256 i = 0; i < 3;) {
             assertTrue(dexMgr.isFunctionApproved(signatures[i]));
+            unchecked { ++i; }
         }

---

320-322: Optimize loop increment for gas efficiency.

For consistency with other similar loops in the file and gas optimization, consider using unchecked increment.

-        for (uint256 i = 0; i < 3; i++) {
+        for (uint256 i = 0; i < 3;) {
             assertFalse(dexMgr.isFunctionApproved(signatures[i]));
+            unchecked { ++i; }
         }

---

239-249: Maintain consistent prank pattern.

For consistency with other test functions, use startPrank/stopPrank pair and maintain consistent order of operations.

-        vm.prank(USER_DIAMOND_OWNER);
+        vm.startPrank(USER_DIAMOND_OWNER);
         dexMgr.addDex(address(c1));
+        vm.stopPrank();

-        vm.stopPrank();
+        vm.startPrank(NOT_DIAMOND_OWNER);
         vm.expectRevert(UnAuthorized.selector);
-        vm.prank(NOT_DIAMOND_OWNER);
         dexMgr.removeDex(address(c1));
+        vm.stopPrank();

---

256-262: Maintain consistent prank pattern.

For consistency with other test functions, use startPrank/stopPrank pair.

-        vm.prank(USER_DIAMOND_OWNER);
+        vm.startPrank(USER_DIAMOND_OWNER);
         dexMgr.batchAddDex(dexs);
+        vm.stopPrank();

+        vm.startPrank(NOT_DIAMOND_OWNER);
         vm.expectRevert(UnAuthorized.selector);
-        vm.prank(NOT_DIAMOND_OWNER);
         dexMgr.batchRemoveDex(dexs);
+        vm.stopPrank();

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: mirooon
PR: lifinance/contracts#975
File: test/solidity/Facets/DeBridgeDlnFacet.t.sol:0-0
Timestamp: 2025-02-17T07:59:54.979Z
Learning: In DeBridgeDlnFacet, chain ID mappings are hardcoded and not designed to be dynamically updated, so tests for mapping updates are not needed.